Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BUGFIX] Contracts stored in state not being updated #1463

Merged
merged 13 commits into from
Mar 18, 2024
Merged

[BUGFIX] Contracts stored in state not being updated #1463

merged 13 commits into from
Mar 18, 2024

Conversation

Da-Colon
Copy link
Contributor

@Da-Colon Da-Colon commented Mar 15, 2024 •
edited
Loading

I'll explain some of the code changes in a review, But basically two things in this PR

Fixes contracts asSigner not properly being stored in state

As a DAO is loaded and stored in state in useGovernanaceContracts, the wallet provider may not be connect to a user's wallet yet and the contract's asSigner was being connected via a FallbackProvider and would not update when the user's connection status settles.

This bug shows it self when attempting to vote and executing.

The Fix

Remove storing Contracts in state. I thought of a few things I could ultimately do, but the best thing was to just save the retrieved address instead state (keeping the basic workflow the same) and as needed, attach the address to the appropriate contract.

Testing

  • Load up a Token DAO. (ERC20 or ER721).
  • Create a proposal
  • Go to the proposal and Refresh the screen on that page
  • Vote (should work just fine, I refresh a few times just to be sure no flakyness)
  • Wait for voting period to end
  • Before Executing, Refresh the browser. Then attempt to execute.

Refreshing on the details screen, when there is a actionable thing to do is best way to show this bug. Take same proposal to
https://app.dev.fractalframework.xyz/ to see this bug.

Closes #1461

Da-Colon added 2 commits March 14, 2024 21:07
@Da-Colon
Fixes Contract state being incorrectly saved into state as FallbackPr…
d2b8b5d 
…ovider

- Saves only addresses to state
@Da-Colon
Support changes to contract state refactor
b2feac2 
- uses useSafeContracts directly instead of global state to get base contracts
@netlify Netlify
Copy link

netlify bot commented Mar 15, 2024 •
edited
Loading

✅ Deploy Preview for fractal-dev ready!

Name Link
🔨 Latest commit 9fba7d4
🔍 Latest deploy log https://app.netlify.com/sites/fractal-dev/deploys/65f88b208ccd3400084ccf45
😎 Deploy Preview https://deploy-preview-1463.app.dev.fractalframework.xyz
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Da-Colon
Da-Colon commented Mar 15, 2024
View reviewed changes
Copy link
Contributor Author

@Da-Colon Da-Colon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ultimately I did my best to keep this at min changes possible. I didn't want to change any current data flow or typing unless I absolutely have to

I left two TODOs in fractal.ts, to spin off from this and update the other places we are doing this (Freeze contracts and TokenClaim Contract). I may do this shortly after this PR once this gets the okay

src/types/fractal.ts Show resolved Hide resolved
src/hooks/DAO/loaders/useGovernanceContracts.ts Show resolved Hide resolved
src/hooks/DAO/loaders/governance/useERC721LinearStrategy.ts Show resolved Hide resolved
@Da-Colon Da-Colon self-assigned this Mar 15, 2024
@Da-Colon Da-Colon added the bug Something isn't working label Mar 15, 2024
@Da-Colon Da-Colon linked an issue Mar 15, 2024 that may be closed by this pull request
BUG: "transaction requires a signer" #1461
Closed
@Da-Colon Da-Colon changed the title [BUGFIX] Contracts Store in state not being updated [BUGFIX] Contracts stored in state not being updated Mar 15, 2024
adamgall
adamgall requested changes Mar 15, 2024
View reviewed changes
Copy link
Member

@adamgall adamgall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work here @Da-Colon!

I've left a bunch of comments / questions / concerns.

Feel free to "resolve" any comments that are just my own ramblings and questions/answers, which don't need a response.

There are some review comments that are questions that I would like a response to.

There are some review comments that call out places that I think are inadvertent mistakes that might need to be fixed.

src/hooks/DAO/loaders/useGovernanceContracts.ts Show resolved Hide resolved
src/hooks/DAO/loaders/useGovernanceContracts.ts Show resolved Hide resolved
src/hooks/DAO/loaders/useGovernanceContracts.ts Show resolved Hide resolved
src/hooks/DAO/loaders/useGovernanceContracts.ts Show resolved Hide resolved
src/hooks/DAO/loaders/useGovernanceContracts.ts Outdated Show resolved Hide resolved
src/hooks/DAO/proposal/useSubmitProposal.ts Show resolved Hide resolved
src/hooks/DAO/proposal/useUserERC721VotingTokens.ts Outdated Show resolved Hide resolved
src/pages/daos/[daoAddress]/proposals/[proposalId]/index.tsx Show resolved Hide resolved
src/providers/App/governanceContracts/reducer.ts Show resolved Hide resolved
src/types/fractal.ts Show resolved Hide resolved
@Da-Colon Da-Colon requested a review from adamgall March 15, 2024 18:36
@adamgall adamgall mentioned this pull request Mar 15, 2024
Closed
adamgall
adamgall approved these changes Mar 15, 2024
View reviewed changes
Copy link
Member

@adamgall adamgall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Giving an approval to this PR after having extensively reviewed and questioned the code, but with the caveat that I have not actually tested the runtime behavior of these changes.

Hoping that others on the team can do some functional testing.

@adamgall adamgall requested a review from tomstuart123 March 18, 2024 17:26
@tomstuart123
Copy link

See link here for why I approved - https://discord.com/channels/839548879216181309/1219322243310555277/1219332979760042045

@adamgall adamgall merged commit abd2889 into develop Mar 18, 2024
7 checks passed
@adamgall adamgall deleted the bugfix/remove-contract-state-00 branch March 18, 2024 18:45
@adamgall adamgall mentioned this pull request Mar 19, 2024
Merged
@adamgall adamgall mentioned this pull request Mar 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomstuart123 tomstuart123 tomstuart123 approved these changes

@adamgall adamgall adamgall approved these changes

@xraystyle1980 xraystyle1980 Awaiting requested review from xraystyle1980

@mudrila mudrila Awaiting requested review from mudrila

@nicolaus-sherrill nicolaus-sherrill Awaiting requested review from nicolaus-sherrill

Assignees

@Da-Colon Da-Colon

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BUG: "transaction requires a signer"
3 participants
@Da-Colon @tomstuart123 @adamgall